Decreases switch statements and deprecates BytesMessageEncoder #250
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
codefromthecrypt
changed the title
codefromthecrypt
force-pushed
the
less-switch
branch
2 times, most recently
from
efaeceb
to
bc45f62
Compare
codefromthecrypt
changed the title
| case JSON: | ||
| return JSON_V2; | ||
| case PROTO3: | ||
| throw new UnsupportedOperationException("PROTO3 is not yet a built-in encoder"); |
There was a problem hiding this comment.
cool thing is that I have a pending change to support this. However, anyone using forEncoding could update into it without prior knowledge.
Signed-off-by: Adrian Cole <adrian@tetrate.io>
codefromthecrypt
force-pushed
the
less-switch
branch
from
152423f
to
3d1d869
Compare
|
seems like spring boot need to release soon, so I will check on this now. hopefully, looks ok to the team, but if you have a chance to take a look, please do! |
anuraaga
approved these changes
Feb 13, 2024
|
Here are some notable diffs. - HttpSender() {
- super(Encoding.JSON);
+ HttpSender(Encoding encoding) {
+ super(encoding);
}
@Override
@@ -74,7 +74,7 @@ abstract class HttpSender extends BytesMessageSender.Base {
if (this.closed) {
throw new ClosedSenderException();
}
- byte[] body = BytesMessageEncoder.JSON.encode(encodedSpans);
+ byte[] body = this.encoding.encode(encodedSpans);
HttpHeaders headers = getDefaultHeaders();
if (needsCompression(body)) {
body = compress(body);
@@ -86,7 +86,7 @@ abstract class HttpSender extends BytesMessageSender.Base {
HttpHeaders getDefaultHeaders() {
HttpHeaders headers = new HttpHeaders();
headers.set("b3", "0");
- headers.set("Content-Type", "application/json");
+ headers.set("Content-Type", this.encoding.mediaType());
return headers;
} @Bean
@ConditionalOnMissingBean(value = MutableSpan.class, parameterizedContainer = BytesEncoder.class)
@ConditionalOnEnabledTracing
- BytesEncoder<MutableSpan> braveSpanEncoder() {
- return new JsonV2Encoder();
+ BytesEncoder<MutableSpan> braveSpanEncoder(Encoding encoding) {
+ return MutableSpanBytesEncoder.forEncoding(encoding);
}
@Bean
@@ -157,8 +158,8 @@ class ZipkinConfigurations {
@Bean
@ConditionalOnMissingBean(value = Span.class, parameterizedContainer = BytesEncoder.class)
@ConditionalOnEnabledTracing
- BytesEncoder<Span> zipkinSpanEncoder() {
- return SpanBytesEncoder.JSON_V2;
+ BytesEncoder<Span> zipkinSpanEncoder(Encoding encoding) {
+ return SpanBytesEncoder.forEncoding(encoding);
} |
reta
approved these changes
Feb 13, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I noticed in our senders and reporters there were multiple internal
switchstatements to get functions for a specific encoding. This is worse externally for zipkin-reporter-brave, as call sites need to duplicate switch statements, and also update them in order to allow another encoding. This is the case with PROTO3, for example, which is not yet supported for brave spans.Here are the main changes:
Encoding.encode(List<byte[]>)from the now deprecatedBytesMessageEncoder.Encoding.mediaType(), useful in internal and external HTTP senders.SpanBytesEncoder.forEncoding(Encoding)to reduce redundant switch logic.MutableSpanBytesEncoder.forEncoding(Encoding)for BraveMutableSpanBytesEncoder.create(Encoding, Tag<Throwable>)for Brave users who want an alt error tag.With these changes in place, external senders such as those defined in spring-boot, can inherit sensible defaults without maintenance.
This acknowledges some specialized senders (such as okhttp) won't use
Encoding.encode(List<byte[]>). However, this change makes commodity senders a lot easier to write and configure, as they can now be driven by justEncodingand an endpoint.